Skip to content

Parallel fixes - #2585

Open
wangcj05 wants to merge 12 commits into
develfrom
parallel-fixes
Open

Parallel fixes#2585
wangcj05 wants to merge 12 commits into
develfrom
parallel-fixes

Conversation

@wangcj05

Copy link
Copy Markdown
Collaborator

Pull Request Description

What issue does this change request address? (Use "#" before the issue to link it, i.e., #42.)
What are the significant changes in functionality due to this change request?
Contents (one commit per patch, messages included)
Patch What it does
0001 Slurm mode: uninitialized __runSbatch crash (B1), <noprecommand>/<nodefile> input handling (B2), checked srun node discovery with scontrol fallback (B6), <nodefileenv> validation
0002 JobHandler: %INDEX1% placeholder was 2*i, now i+1 (B3)
0003 JobHandler: Dask shutdown guard used the Ray flag; remote/local Dask worker teardown; Ray 2.x debug guards (B4)
0004 RayRunner.kill/DaskRunner.kill actually cancel the remote task (B5)
0005 Explicit runSucceeded flag: legit None returns aren't failures; failure tracebacks propagated to logs and failed-job metadata (B7, #7)
0006 SharedMemoryRunner: bounded thread-kill loop (B8)
0007 RemoteNodeScripts: --help exit, START_OUTPUT, --num-gpus forwarding, quoting, optional bash profile (B9)
0008 PBS/MPI-legacy: clear error for missing <nodefileenv> env var (B10)
0009 Shared ClusterSimulationMode base + ClusterUtils module; Slurm/PBS refactored; unified job-name rules; 21+ unit tests (#8)
0010 Slurm <useSrun/> native launch mode (#9)
0011 <daskJobqueue> RunInfo option using dask-jobqueue SLURMCluster/PBSCluster (#10)

For Change Control Board: Change Request Review

The following review must be completed by an authorized member of the Change Control Board.

  • 1. Review all computer code.
  • 2. If any changes occur to the input syntax, there must be an accompanying change to the user manual and xsd schema. If the input syntax change deprecates existing input files, a conversion script needs to be added (see Conversion Scripts).
  • 3. Make sure the Python code and commenting standards are respected (camelBack, etc.) - See on the wiki for details.
  • 4. Automated Tests should pass, including run_tests, pylint, manual building and xsd tests. If there are changes to Simulation.py or JobHandler.py the qsub tests must pass.
  • 5. If significant functionality is added, there must be tests added to check this. Tests should cover all possible options. Multiple short tests are preferred over one large test. If new development on the internal JobHandler parallel system is performed, a cluster test must be added setting, in XML block, the node <internalParallel> to True.
  • 6. If the change modifies or adds a requirement or a requirement based test case, the Change Control Board's Chair or designee also needs to approve the change. The requirements and the requirements test shall be in sync.
  • 7. The merge request must reference an issue. If the issue is closed, the issue close checklist shall be done.
  • 8. If an analytic test is changed/added is the the analytic documentation updated/added?
  • 9. If any test used as a basis for documentation examples (currently found in raven/tests/framework/user_guide and raven/docs/workshop) have been changed, the associated documentation must be reviewed and assured the text matches the example.

wangcj05 and others added 12 commits August 24, 2026 09:05
…afe node discovery (B1,B2,B6,B10-slurm)

- Initialize self.__runSbatch = False in __init__ so remoteRunCommand() no
  longer raises AttributeError when <runSbatch> is absent (B1).
- Declare <nodefile>/<nodefileenv> in the input specification and handle all
  option names case-insensitively; <noprecommand> previously never matched the
  camelCase comparison in handleInput (B2). Unknown options now warn.
- <nodefileenv> validates the environment variable exists (B10).
- Replace unchecked 'os.system(srun ... > file)' with a checked subprocess
  call, plus a 'scontrol show hostnames' + SLURM_TASKS_PER_NODE fallback in
  the new ClusterUtils module; empty node files now raise a clear error (B6).
- Close node file handles via context managers; replace debug print() with
  message-handler calls; fix stale 'Not in PBS' comment.
kwargs['INDEX1'] was computed as str(i+i) (i.e. 2*i) instead of the intended
1-based batch-slot index str(i+1). Any code-interface command using the
%INDEX1% placeholder received 0,2,4,... for slots 0,1,2,..., silently
selecting the wrong node files / bindings for batch slots >= 1.
- The Dask branch of __shutdownParallel guarded on rayInstanciatedOutside
  instead of daskInstanciatedOutside; with an externally provided scheduler
  RAVEN could tear down resources it does not own.
- When RAVEN owns the cluster, call Client.shutdown() so the scheduler retires
  ALL workers (including the ssh-launched remote 'dask worker' processes,
  which previously had no teardown path and were left as zombies), then
  terminate the tracked head-node worker and scheduler subprocesses.
- Track the head-node dask worker Popen (previously a dropped local variable).
- Close ray_head.ip and dask worker log file handles via context managers.
- Guard Ray address_info debug prints with .get(); 'redis_address' was
  removed in Ray 2.x and raised KeyError under verbose debugging.
RayRunner.kill() and DaskRunner.kill() only dropped the local ObjectRef /
Future, so 'killed' remote tasks kept executing and consuming cluster cores
(affecting optimizer early stopping, terminateJobs and terminateAll). Now
kill() issues ray.cancel(force=True, recursive=True) / Future.cancel() before
releasing the reference, with a warning if cancellation fails.
…; propagate tracebacks (B7, quick-ref #7)

- InternalRunner gains an explicit tri-state runSucceeded flag and a
  failureInfo string (formatted traceback / RayTaskError text), exposed via
  getFailureInfo(). getEvaluation() now returns Error() based on the explicit
  flag; the 'None return == failure' sentinel remains only as a legacy
  fallback when no outcome was recorded.
- SharedMemoryRunner: the bare lambda thread target is replaced by a wrapper
  that catches exceptions, records the traceback and the outcome; functions
  that legitimately return None are no longer marked failed.
- RayRunner/DaskRunner: success and failure are recorded explicitly on
  ray.get()/Future.result(); failure text is kept. DaskRunner no longer
  re-raises inside _collectRunnerResponse, so getEvaluation() cannot blow up
  the collecting thread (getReturnCode retains its failure semantics).
- JobHandler.__checkAndRemoveFinished logs the failure details and stores them
  under 'failureInfo' in the failed-job metadata.
PyThreadState_SetAsyncExc is ignored while a thread is blocked in C
extensions or system calls, so the previous 'while alive: sleep; kill()' loop
could spin forever. kill() now retries for a bounded 10 s window, then warns
and abandons the (daemon) thread, marking the run as failed explicitly.
…s, quote variables (B9)

- start_remote_servers.sh: '--help' used 'return' which is invalid in an
  executed script (now 'exit 0'); error paths exit non-zero; START_OUTPUT was
  referenced by tee but never defined (now <output>_start.log); the parsed
  --num-gpus value was silently dropped and is now forwarded to start_ray.sh;
  variable expansions are quoted.
- start_ray.sh: new NUM_GPUS positional argument, passed to 'ray start
  --num-gpus' only when non-negative; optional bash profile; quoted variables.
- start_dask.sh: the remote bash profile is now optional (previously
  'source ""' failed when unset); 'cd' is checked; quoted variables.
…le (B10)

PBS and MPI-legacy modes read os.environ[<nodefileenv>] directly, producing a
raw KeyError traceback when the variable is undefined. Both now raise a RAVEN
IOError naming the missing variable.
…, add unit tests (quick-ref #8)

- New ClusterMode.ClusterSimulationMode base class holds the previously
  triplicated modifyInfo logic: node-file reading, batch-size clamping,
  per-batch node_%INDEX% file splitting, mpiexec precommand assembly and
  thread postcommand handling. Slurm and PBS modes are now thin subclasses
  that only perform scheduler-specific node discovery and submission (the MPI
  legacy mode is intentionally left untouched).
- ClusterUtils gains pure, framework-independent functions (readNodeFile,
  computeBatchSize, writeNodeSubFiles, buildMPIPrecommand, sanitizeJobName)
  used by the base class.
- Job-name validation is unified via ClusterUtils.sanitizeJobName: both modes
  now accept alphanumerics, '_' and '-' (the Slurm mode previously rejected
  '-' while PBS allowed it), PBS keeps its 15-character truncation, and both
  raise RAVEN errors instead of bare IOError/print.
- Empty PBS node files now raise a clear error (previously silently produced
  batchSize=1); PBS node-file handles are context-managed via readNodeFile.
- New standalone unit tests (21 cases) in CustomModes/tests/, runnable
  without a built RAVEN: python3 ravenframework/CustomModes/tests/test_cluster_utils.py
New optional <useSrun/> mode option: run precommands become
'srun --overlap --exact -n <NumMPI> [MPIParam...]' instead of
'mpiexec <NodeParameter> <nodefile> -n <NumMPI>'. Slurm assigns per-step
resources itself, so the per-batch node_%INDEX% files are skipped entirely,
removing the hostfile-flag dependence on the MPI flavor (MPICH -f vs OpenMPI
--hostfile) and working with all major MPI stacks via PMI/PMIx. Node
discovery is kept so runInfo['Nodes'] remains available for internal-parallel
cluster bring-up. Extra <MPIParam> entries are passed to srun (e.g.
--mpi=pmix). Includes unit tests for the precommand construction.
…ref #10)

New <daskJobqueue> RunInfo element (used with <internalParallel>True
</internalParallel> and <parallelMethod>dask</parallelMethod>): the element
text selects the scheduler ('slurm' or 'pbs') and the XML attributes are
passed to the dask_jobqueue SLURMCluster/PBSCluster constructor. Example:

  <daskJobqueue memory="4GB" queue="short" account="proj1">slurm</daskJobqueue>

Dask workers are then submitted AS scheduler jobs by dask_jobqueue, removing
the inter-node-ssh requirement and the hand-rolled bring-up scripts for this
path, with clean teardown (client + cluster close cancels the worker jobs).
'memory' is required (dask_jobqueue mandates it); 'cores' defaults to
numProcByRun, 'jobs' (number of worker scheduler jobs) defaults to batchSize,
'walltime' defaults to <expectedTime>; other attributes pass through
verbatim. Fails with a clear error when dask_jobqueue is not installed.
Kwargs assembly lives in ClusterUtils.assembleDaskJobqueueKwargs (pure,
covered by unit tests). The cluster object is excluded from pickling.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant